Skip to content

Pass error/meta to case reducers, support non-FSAs #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Pass error/meta to case reducers, support non-FSAs #9

wants to merge 6 commits into from

Conversation

hnordt
Copy link
Contributor

@hnordt hnordt commented Mar 6, 2018

I would like to suggest that we pass error/meta to case reducers:

function myCaseReducer(state, payload, { error, meta }) {
    // ...
}

It might be useful.

Also I've added support to non-FSA.

Building is now failing, I don't know how to add support for rest spread in Rollup. Alternatively I could change the code to use omit():

const {type, payload} = action;
const rest = omit(action, ["type", "payload"])

@nickserv
Copy link
Collaborator

nickserv commented Mar 6, 2018

Nice, I like that we're using the FSA package for consistency with other tooling.

I would like to suggest that we pass error/meta to case reducers:

Does this change actually cause the FSA package to pass error and meta to the reducer or is this something the user would set up manually in action creators?

Building is now failing, I don't know how to add support for rest spread in Rollup.

Rest spread is supported in Node 8.6.0 (active LTS), so I'd recommend updating Node and not worrying about it. We could tweak babel-preset-env to compile against older versions of Node when we need to, or you can do it if you can't update Node for some reason.

@hnordt
Copy link
Contributor Author

hnordt commented Mar 6, 2018

@nickmccurdy the FSA package is just used for checking if the action is a FSA.

I'm passing the error and meta to the case reducer so you can use that info to transform state inside the case reducer:

const addTodoSuccess = todo => ({ type: ADD_TODO, payload: todo })

const addTodoFailure = error => ({ type: ADD_TODO, payload: error, error: true })

// here we don't need to read error or meta, just payload
const addTodoReducer = (state, todo) => state.push({ ...todo, completed : false })

// here we need to read error
const notificationsReducer = (state, payload, { error }) =>
  error ? state.push({ type: "danger", message: payload.message }) : state

The reason is that when error is true, payload could be anything, new Error() is just a convention, which might not be followed. Also the user might want to read meta.

@nickserv
Copy link
Collaborator

nickserv commented Mar 6, 2018

That makes sense, would you mind documenting that this convention can be optionally used in the readme?

@hnordt
Copy link
Contributor Author

hnordt commented Mar 6, 2018

@nickmccurdy I've updated the createReducer section in README. Please review it, you might want to improve the wording.

@nickserv
Copy link
Collaborator

nickserv commented Mar 6, 2018

I like your wording and the code examples, but I find the difference between the first two examples a little confusing. They both use FSAs now so maybe they should be merged since there's not much different between them. We should also stay consistent and use immer-style mutating reducers (unlike the FSA docs) because that's unrelated to how we implement reducers with or without FSAs.

@hnordt
Copy link
Contributor Author

hnordt commented Mar 6, 2018

@nickmccurdy I've merged first two examples. I removed the returns, I'm not familiar with immer, so you might want to review.

Copy link
Collaborator

@nickserv nickserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's better. 😄 I'm fine with this assuming there was some basic manual testing involved, haven't tried it myself self.

@markerikson
Copy link
Collaborator

Eh, I'm inclined to say we just switch back to a standard (state, action) signature rather than (state, payload). I guess if people want to grab just the payload in their reducer, they can always destructure it like function myReducer(state, {payload})

@markerikson
Copy link
Collaborator

I'll close this PR, but please go ahead and submit one that changes createReducer to just pass the whole action instead.

@markerikson markerikson closed this Mar 7, 2018
phryneas pushed a commit that referenced this pull request Nov 2, 2021
* Add a generator for react hooks option
* Add prettier, parse URLs and files from CLI

Co-authored-by: kahirokunn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants